Skip to content

fix(markers): apply lazy evaluation to markers #877

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

frostming
Copy link
Contributor

@frostming frostming commented Feb 25, 2025

Signed-off-by: Frost Ming [email protected]

Fix #774

Change the marker evaluation to lazy evaluation so it is possible for boolean shortcuts. I didn't test but there should also be a performance gain.

@wimglenn
Copy link

Is this different to #834 ?

@frostming
Copy link
Contributor Author

frostming commented Feb 25, 2025

@wimglenn Thank you, I just noticed this PR. In fact, #834 is still eager, but it allows exceptions to exist and considers them during evaluation. The implementation also doesn't look so clean

By making the evaluation lazy there would be a minor breaking change that illegal markers will be missed if being shortcutted.

@edwardpeek-crown-public
Copy link

FWIW PR #834 started off as a simple lazy evaluation like this but discussion on issue #774 indicated an aversion to the edge cases this approach introduces with order dependency and silently ignoring malformed expressions.

@notatallshaw
Copy link
Member

notatallshaw commented Jun 3, 2025

Yes, the packaging maintainers need to give clear guidance here, I think we have narrowed down to there only being 4 options on solving the practical problem users are facing:

  1. Implement the standard that there should be a fallback to Python string comparison
  2. Implement lazy comparison and accept that markers could be invalid when not evaluated
  3. Do a pass to check markers are valid and then do a lazy comparison
  4. Refuse all changes until spec is changed or clarified

If the answer is 4 I am willing to start the discussion on clarifying the spec, specifically the two separate standards topics: packaging rejecting solution 1. And whether the spec allows for 2 or 3.

I do not have the capacity to propose a new solution, but I am willing to explain the practical issues and put forth the already outlined options to see if there is a consensus in the Python packaging community.

@frostming
Copy link
Contributor Author

frostming commented Jun 3, 2025

silently ignoring malformed expressions.

This is not true, malformed expressions will be rejected by the parser and are never caught by the evaluation.

@notatallshaw
Copy link
Member

silently ignoring malformed expressions.

This is not true, malformed expressions will be rejected by the parser and are never caught by the evaluation.

Can you please add additional tests for this, assuming they don't exist already.

My experience working on pre-releaes is packaging is far from exhaustively tested on edge cases.

@frostming frostming force-pushed the fix/lazy-evaluation branch from f95379c to aada1f2 Compare June 3, 2025 01:25
@edwardpeek-crown-public
Copy link

This is not true, malformed expressions will be rejected by the parser and are never caught by the evaluation.

I should have been more specific, but I meant this repo's non-standard error in cases of "bad" version comparisons.

@frostming frostming force-pushed the fix/lazy-evaluation branch from a1450a6 to 2e4b85b Compare June 3, 2025 01:38
@frostming
Copy link
Contributor Author

I should have been more specific, but I meant this repo's non-standard error in cases of "bad" version comparisons.

IIUC you mean os_name ~= "2", then yes it isn't caught by the parser. But that is also an implementation detail and should be clarified by the spec IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Marker referencing 'platform_release' fails to evaluate on Linux systems with non-PEP 440 kernel versions
4 participants